Patches to help build node as a shared library for Android #29388

gcampax · 2019-08-31T22:29:17Z

Hello! I maintain an Android app that uses nodejs as a library. These patches have been necessary to build.
The patches are tested on the 8.x branch, which we're using currently. They did not rebase cleanly on master because the corresponding core was moved around, but I think they are small enough they should be correct.

I hope the patches are generally useful. I would love to have them upstream, so I can move away from 8.x

devnexen · 2019-08-31T23:34:10Z

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

Trott · 2019-09-01T00:31:30Z

@gcampax Thank you so much! Improving Android support has been a wish-list item for a long time.

@nodejs/build @nodejs/build-files @nodejs/gyp

Trott · 2019-09-01T00:32:22Z

@nodejs/python

Trott · 2019-09-01T00:33:14Z

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

If you end up rebasing or something, fixing the commit message would be nice, but if not, whoever lands this will fix it at that time, so it's not critical. But if you're into saving someone else a few keystrokes, yeah, please follow those guidelines!

gcampax · 2019-09-01T05:56:06Z

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

Of course! Thanks for the review.
I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

configure.py

devnexen · 2019-09-01T08:44:14Z

Hello and thanks for your interest !
The commit messages need to follow this guideline https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines

Of course! Thanks for the review.
I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

The first line should:...be prefixed with the name of the changed subsystem means nodejs project structure subsystem.

bnoordhuis · 2019-09-01T10:38:29Z

common.gypi

-            'cflags': [ '-fPIE' ],
-            'ldflags': [ '-fPIE', '-pie' ]
+            'cflags': [ '-fPIC' ],
+            'ldflags': [ '-fPIC' ]


You only build the shared library, don't you? Does this also work when building the node executable? -fPIE means Position Independent Executable.

It will work, because you can build an executable with PIC code, but it will not be as efficient as PIE code. Not sure about the -pie link flag though.

Can you check that it still produces a working binary? I have no way to test myself.

configure.py

Trott · 2019-09-01T17:06:24Z

I thought I did follow the guidelines though: the commit summary is prefixed with the subsystem name, is imperative, and is lowercase; the body is wrapped at 72 characters (approx.). Anything else I should fix?

The subsystem should be build: rather than android: but that's a super minor thing. While there are some guidelines for how to determine the subsystem, a lot of it basically comes down to "This is how it's been done for the last several years."

nodejs-github-bot · 2019-09-01T17:14:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25313/

Trott · 2019-09-07T04:06:21Z

@nodejs/collaborators @nodejs/hardware Pinging widely on this, sorry, but if anyone is invested in Node.js running on Android (and even if not), this could use some reviews. Even better, it could use some testing--that is, someone who can say "I tried to compile with this on Android and it worked!"

devnexen · 2019-09-09T15:35:13Z

Would it be handy (and useful) to update android-configure ?

Trott · 2019-09-26T04:31:52Z

@nodejs/members Anyone able to try compiling on Android with this patch and report results? It would be super-cool to get some confirmation that it works for people and inch Android support a little bit forward.

gengjiawen · 2019-09-26T04:44:14Z

@gcampax what's your step to build Node.js as a android library ?

gcampax · 2019-09-26T17:04:13Z

Hi all, sorry for the delayed reply...

@gcampax what's your step to build Node.js as a android library ?

As I mentioned at the openining of the issue, I build the 8.x branch, not master, and I forward-ported my patches without really testing them.
On 8.x, I set CC, CXX, LINK, etc. environment variables as well as CC.host, CXX.host, etc. and then I run:

export GYP_DEFINES="target_arch=${arch} v8_target_arch=${nodejs_cpu} android_target_arch=${arch} host_os=linux OS=android"
./configure --shared --dest-cpu=${nodejs_cpu} --dest-os=android --without-snapshot --openssl-no-asm --with-intl=small-icu --without-inspector

You can find the full build script at https://github.com/stanford-oval/almond-android/blob/master/rebuild-libraries.sh , including the appropriate settings of arch/android_arch/nodejs_cpu .

gireeshpunathil · 2019-10-19T14:21:11Z

just pinging folks in general to see where we stand. Did anyone have built / tested this successfully? I know our CI will not complain, as there is no flow that captures the android build and test.

gireeshpunathil · 2019-10-19T14:23:18Z

the Travis CI failure is because wrong sub-system. There seem to be an android sub-system, but the commit message validator seem to have no knowledge about it.

either we change the subsystem to shared_lib, or ignore error for now?

richardlau · 2019-10-19T17:17:22Z

the Travis CI failure is because wrong sub-system. There seem to be an android sub-system, but the commit message validator seem to have no knowledge about it.

either we change the subsystem to shared_lib, or ignore error for now?

https://github.com/nodejs/node/pull/29388/commits#issuecomment-526936711

gireeshpunathil · 2019-10-21T04:48:45Z

@richardlau - sorry, I did not follow that. I meant to say: android as a label exists, but not as a module sub-system, or at least the commit metadata validator does not recognize that; and hence the linter failure. Do you agree?

richardlau · 2019-10-21T05:08:19Z

@richardlau - sorry, I did not follow that. I meant to say: android as a label exists, but not as a module sub-system, or at least the commit metadata validator does not recognize that; and hence the linter failure. Do you agree?

Although there is some overlap, labels and subsystems are not quite the same thing. For example, currently the only platform subsystem that core-validate-commit recognises is win while the equivalent label is windows.

Of the currently recognised subsystems recognised by the validator build would seem to be the most fitting, as suggested by @Trott earlier.

gireeshpunathil · 2019-10-21T05:23:49Z

pinging @gcampax to see where do we stand and what it takes to move this forward.

Trott · 2019-10-22T03:30:25Z

pinging @gcampax to see where do we stand and what it takes to move this forward.

There are a few comments above for @gcampax but for the most part, the hold up here would seem to be a lack of Android folks among Collaborators who can test and/or approve this. Maybe one of our Google folks might be able to identify someone who might have an interest in Node.js on Android who can help out with testing and/or review? @fhinkel @MylesBorins @ofrobots @bcoe @bmeurer @eugeneo @psmarshall @hashseed

Compiling a library with -fPIE won't do, and on Android libraries are not versioned.

And other errors like lost promises

gcampax · 2019-10-22T06:18:44Z

Ok, I have force-pushed an update of this patchset. As I mentioned, I don't have a way to test master, because I am currently building 8.x and every version switch is a lot of work.

nodejs-github-bot · 2019-12-02T16:40:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27276/

gireeshpunathil · 2019-12-09T06:56:40Z

the CI is green; is there anything else that needs to be done before this can get in? @bcoe - was there any insights / comments that came out of the internal review you mentioned earlier? I would like to land this, but not being an SME on this area, would like to hear a green signal from one more expert!

Trott

Rubber-stamp LGTM. If you're the only Android dev weighing in, then let's at least make your life easier.

Trott · 2019-12-12T14:10:50Z

Landed in d8fc0ae...73df09e

Compiling a library with -fPIE won't do, and on Android libraries are not versioned. PR-URL: nodejs#29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

And other errors like lost promises PR-URL: nodejs#29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Compiling a library with -fPIE won't do, and on Android libraries are not versioned. PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

And other errors like lost promises PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Compiling a library with -fPIE won't do, and on Android libraries are not versioned. PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

And other errors like lost promises PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Compiling a library with -fPIE won't do, and on Android libraries are not versioned. PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

And other errors like lost promises PR-URL: #29388 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>

nodejs/node#29388

* chore: bump node in DEPS to v12.16.0 * Fixup asar support setup patch nodejs/node#30862 * Fixup InternalCallbackScope patch nodejs/node#30236 * Fixup GN buildfiles patch nodejs/node#30755 * Fixup low-level hooks patch nodejs/node#30466 * Fixup globals require patch nodejs/node#31643 * Fixup process stream patch nodejs/node#30862 * Fixup js2c modification patch nodejs/node#30755 * Fixup internal fs override patch nodejs/node#30610 * Fixup context-aware warn patch nodejs/node#30336 * Fixup Node.js with ltcg config nodejs/node#29388 * Fixup oaepLabel patch nodejs/node#30917 * Remove redundant ESM test patch nodejs/node#30997 * Remove redundant cli flag patch nodejs/node#30466 * Update filenames.json * Remove macro generation in GN build files nodejs/node#30755 * Fix some compilation errors upstream * Add uvwasi to deps nodejs/node#30258 * Fix BoringSSL incompatibilities * Fixup linked module patch nodejs/node#30274 * Add missing sources to GN uv build libuv/libuv#2347 * Patch some uvwasi incompatibilities * chore: bump Node.js to v12.6.1 * Remove mark_arraybuffer_as_untransferable.patch nodejs/node#30549 * Fix uvwasi build failure on win * Fixup --perf-prof cli option error * Fixup early cjs module loading * fix: initialize diagnostics properly nodejs/node#30025 * Disable new esm syntax specs nodejs/node#30219 * Fixup v8 weakref hook spec nodejs/node#29874 * Fix async context timer issue * Disable monkey-patch-main spec It relies on nodejs/node#29777, and we don't override prepareStackTrace. * Disable new tls specs nodejs/node#23188 We don't support much of TLS owing to schisms between BoringSSL and OpenSSL. Co-authored-by: Shelley Vohr <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 31, 2019

Trott added the android Issues and PRs related to the android platform. label Sep 1, 2019

cclauss reviewed Sep 1, 2019

View reviewed changes

configure.py Show resolved Hide resolved

bnoordhuis reviewed Sep 1, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

gcampax added 2 commits October 21, 2019 23:14

build: fix library version and compile flags on Android

77c2273

Compiling a library with -fPIE won't do, and on Android libraries are not versioned.

build: on Android, use android log library to print stack traces

f18b9da

And other errors like lost promises

gcampax force-pushed the android-patches branch from f63026b to f18b9da Compare October 22, 2019 06:15

cclauss approved these changes Dec 9, 2019

View reviewed changes

benjamingr approved these changes Dec 9, 2019

View reviewed changes

aymen94 approved these changes Dec 9, 2019

View reviewed changes

Trott approved these changes Dec 12, 2019

View reviewed changes

Trott closed this Dec 12, 2019

MylesBorins mentioned this pull request Dec 13, 2019

v13.4.0 proposal #30937

Merged

gireeshpunathil mentioned this pull request Dec 30, 2019

Working build script for Android ARM Node 7.x or 8.x shared library #14783

Closed

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020

Fixup Node.js with ltcg config

367e74c

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 12, 2020

Fixup Node.js with ltcg config

3b1c4ba

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 15, 2020

Fixup Node.js with ltcg config

d841a4e

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 18, 2020

Fixup Node.js with ltcg config

a825594

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020

Fixup Node.js with ltcg config

0078ca3

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 21, 2020

Fixup Node.js with ltcg config

8299d10

nodejs/node#29388

codebytere added a commit to electron/electron that referenced this pull request Feb 24, 2020

Fixup Node.js with ltcg config

b726509

nodejs/node#29388

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patches to help build node as a shared library for Android #29388

Patches to help build node as a shared library for Android #29388

gcampax commented Aug 31, 2019

devnexen commented Aug 31, 2019

Trott commented Sep 1, 2019

Trott commented Sep 1, 2019

Trott commented Sep 1, 2019

gcampax commented Sep 1, 2019

devnexen commented Sep 1, 2019

bnoordhuis Sep 1, 2019

gcampax Sep 2, 2019

bnoordhuis Sep 2, 2019

Trott commented Sep 1, 2019

nodejs-github-bot commented Sep 1, 2019

Trott commented Sep 7, 2019

devnexen commented Sep 9, 2019

Trott commented Sep 26, 2019

gengjiawen commented Sep 26, 2019

gcampax commented Sep 26, 2019

gireeshpunathil commented Oct 19, 2019

gireeshpunathil commented Oct 19, 2019

richardlau commented Oct 19, 2019

gireeshpunathil commented Oct 21, 2019

richardlau commented Oct 21, 2019

gireeshpunathil commented Oct 21, 2019

Trott commented Oct 22, 2019

gcampax commented Oct 22, 2019

nodejs-github-bot commented Dec 2, 2019

gireeshpunathil commented Dec 9, 2019

Trott left a comment

Trott commented Dec 12, 2019

Patches to help build node as a shared library for Android #29388

Patches to help build node as a shared library for Android #29388

Conversation

gcampax commented Aug 31, 2019

devnexen commented Aug 31, 2019

Trott commented Sep 1, 2019

Trott commented Sep 1, 2019

Trott commented Sep 1, 2019

gcampax commented Sep 1, 2019

devnexen commented Sep 1, 2019

bnoordhuis Sep 1, 2019

Choose a reason for hiding this comment

gcampax Sep 2, 2019

Choose a reason for hiding this comment

bnoordhuis Sep 2, 2019

Choose a reason for hiding this comment

Trott commented Sep 1, 2019

nodejs-github-bot commented Sep 1, 2019

Trott commented Sep 7, 2019

devnexen commented Sep 9, 2019

Trott commented Sep 26, 2019

gengjiawen commented Sep 26, 2019

gcampax commented Sep 26, 2019

gireeshpunathil commented Oct 19, 2019

gireeshpunathil commented Oct 19, 2019

richardlau commented Oct 19, 2019

gireeshpunathil commented Oct 21, 2019

richardlau commented Oct 21, 2019

gireeshpunathil commented Oct 21, 2019

Trott commented Oct 22, 2019

gcampax commented Oct 22, 2019

nodejs-github-bot commented Dec 2, 2019

gireeshpunathil commented Dec 9, 2019

Trott left a comment

Choose a reason for hiding this comment

Trott commented Dec 12, 2019